Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exploratory work towards a potential v3 release #36

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

mroth
Copy link
Owner

@mroth mroth commented Feb 7, 2024

Key breaking changes would include (TBD list in progress):

Non-breaking changes:

This a first step in refactoring towards a v3 release.  A major semantic
version release is required in order to let us fully remove the
deprecated PickSource from our API.

Switches to using the new math/rand/v2 module.  Paving the way for the
future.

Removes PickSource method: As planned, this will remove the previously
deprecated PickSource method that uses v1 of math/rand sources. Custom
randomness source should be reintroduced in a future version using a
different methodology (e.g. setting on the Chooser instead of with each
function call).

Switches the Chooser backing from being a system int to a uint64. This
should result in defined behavior across 32 and 64 bit systems (with
the potential for some performance regressions on 32 bit systems, which
I consider an acceptable tradeoff).

Internal implementation: removes the custom searchInts binary sort that
was present for performance (ala github.com/mroth/xsort)in favor of
slices.BinarySearch which is available as of go1.21 and hits acceptable
performance as of go1.22.

goos: darwin
goarch: arm64
pkg: github.com/mroth/weightedrand/v2
                              │ v2-main.txt  │             v3-dev1.txt              │
                              │    sec/op    │    sec/op     vs base                │
NewChooser/size=1e1-8           132.7n ±  0%   132.9n ±  0%       ~ (p=0.314 n=6)
NewChooser/size=1e2-8           476.1n ±  1%   472.8n ±  0%  -0.70% (p=0.002 n=6)
NewChooser/size=1e3-8           3.406µ ±  0%   3.412µ ±  0%       ~ (p=0.379 n=6)
NewChooser/size=1e4-8           31.19µ ±  1%   31.03µ ±  0%  -0.51% (p=0.002 n=6)
NewChooser/size=1e5-8           296.6µ ±  0%   295.9µ ±  0%       ~ (p=0.394 n=6)
NewChooser/size=1e6-8           2.843m ±  1%   2.843m ±  1%       ~ (p=0.485 n=6)
NewChooser/size=1e7-8           35.83m ±  1%   35.92m ±  1%       ~ (p=0.485 n=6)
Pick/size=1e1-8                 22.49n ±  8%   20.28n ±  9%  -9.80% (p=0.015 n=6)
Pick/size=1e2-8                 35.26n ±  2%   32.82n ±  2%  -6.92% (p=0.002 n=6)
Pick/size=1e3-8                 48.41n ±  1%   45.38n ±  1%  -6.26% (p=0.002 n=6)
Pick/size=1e4-8                 63.30n ±  1%   60.23n ±  2%  -4.85% (p=0.002 n=6)
Pick/size=1e5-8                 85.92n ±  1%   82.53n ±  1%  -3.95% (p=0.002 n=6)
Pick/size=1e6-8                 111.5n ±  1%   107.3n ±  4%  -3.72% (p=0.013 n=6)
Pick/size=1e7-8                 240.7n ±  2%   233.2n ±  1%  -3.10% (p=0.002 n=6)
PickParallel/size=1e1-8         2.982n ±  6%   2.760n ±  5%  -7.43% (p=0.009 n=6)
PickParallel/size=1e2-8         4.679n ±  1%   4.360n ±  2%  -6.81% (p=0.002 n=6)
PickParallel/size=1e3-8         6.422n ±  2%   6.059n ±  1%  -5.66% (p=0.002 n=6)
PickParallel/size=1e4-8         8.463n ±  0%   8.114n ± 58%       ~ (p=0.058 n=6)
PickParallel/size=1e5-8         11.55n ±  3%   11.06n ±  0%  -4.24% (p=0.002 n=6)
PickParallel/size=1e6-8         14.98n ±  0%   14.40n ±  0%  -3.87% (p=0.002 n=6)
PickParallel/size=1e7-8         34.70n ±  0%   33.71n ±  0%  -2.82% (p=0.002 n=6)
PickSourceParallel/size=1e1-8   2.752n ± 10%
PickSourceParallel/size=1e2-8   4.369n ±  2%
PickSourceParallel/size=1e3-8   5.989n ±  1%
PickSourceParallel/size=1e4-8   7.991n ±  2%
PickSourceParallel/size=1e5-8   11.28n ±  0%
PickSourceParallel/size=1e6-8   14.59n ±  0%
PickSourceParallel/size=1e7-8   33.86n ±  0%
geomean                         120.0n         279.7n        -3.59%               ¹
¹ benchmark set differs from baseline; geomeans may not be comparable
Since we're already requiring the slices package in stdlib with this
refactor, we can utilize this newer function which should be slightly
more efficient (and has nicer ergonomics imo).

Performs roughly ~11% faster during NewChooser initialization.

goos: darwin
goarch: arm64
pkg: github.com/mroth/weightedrand/v2
                        │  v3-dev1.txt  │             v3-dev2.txt              │
                        │    sec/op     │   sec/op     vs base                 │
NewChooser/size=1e1-8     132.90n ±  0%   70.73n ± 1%  -46.78% (p=0.002 n=6)
NewChooser/size=1e2-8      472.8n ±  0%   444.1n ± 2%   -6.05% (p=0.002 n=6)
NewChooser/size=1e3-8      3.412µ ±  0%   3.333µ ± 0%   -2.30% (p=0.002 n=6)
NewChooser/size=1e4-8      31.03µ ±  0%   30.30µ ± 0%   -2.33% (p=0.002 n=6)
NewChooser/size=1e5-8      295.9µ ±  0%   291.9µ ± 1%   -1.36% (p=0.002 n=6)
NewChooser/size=1e6-8      2.843m ±  1%   2.775m ± 1%   -2.37% (p=0.002 n=6)
NewChooser/size=1e7-8      35.92m ±  1%   32.99m ± 2%   -8.16% (p=0.002 n=6)
geomean                    279.7n         36.41µ       -11.60%               ¹
¹ benchmark set differs from baseline; geomeans may not be comparable
The internal mechanics of this are a bit inelegant, since unfortunately
the global randomness source is not exported, necessitating these nil
check methods instead.

The API here needs some user feedback. I believe the majority case will
want to set this once and not on a per-call basis (cf. the deprecated
PickSource method in the previous version), but that needs be validated.
@trungdlp-wolffun
Copy link

I'm very excited to improve. Can I help you, @mroth?

@mroth
Copy link
Owner Author

mroth commented Aug 16, 2024

I'm very excited to improve. Can I help you, @mroth?

That'd be great! Now that Go 1.23 is released, mostly what remains here is validating the new method for setting a custom randomness source, seen in cd42f24. I'm still not sure about the API here.

@robinjoseph08
Copy link

As a user of v2, I think the API looks good here. As a bonus, it would be nice to rename your NewChooser function to NewChooserWithRand and then make a new simple NewChooser function that just calls NewChooserWithRand with nil as the passed in rand value. I think most people's use-cases will be wanting to set the rand at instantiation, and while you can do it with

c, _ := weightedrand.NewChooser(...)
c.SetRand(r)

it's more convenient to do it all in a single call.

If you're on the fence about including the SetRand method altogether, I personally don't have a major use for it, though I could see it being useful for testing purposes (i.e. swapping out the rand source for each test so that various cases can be tested). Maybe you can leave it off for the first version of v3 (assuming you add the NewChooserWithRand function), and if you get requests for it, you can easily add it back in? It's harder to deprecate/remove functionality than to add it.

Other than that, it all seems reasonable to me, and I'm excited for when it's released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants